Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent new tag from being overwritten #4385

Closed
wants to merge 3 commits into from
Closed

Prevent new tag from being overwritten #4385

wants to merge 3 commits into from

Conversation

glinton
Copy link
Contributor

@glinton glinton commented Jul 5, 2018

This prevents tags from being overwritten by other regex processors.

Resolves #4374

@danielnelson danielnelson added this to the 1.7.2 milestone Jul 5, 2018
@danielnelson danielnelson added the fix pr to fix corresponding bug label Jul 5, 2018
@danielnelson
Copy link
Contributor

It appears from the test case that this behavior was added on purpose. @44px do you have a specific use case where you need this behavior?

I think it makes sense to prevent empty tag/fields keys and tag values, though I'm less sure about field values. It's imaginable that someone would want an empty string as a field value, but I imagine it is almost always unwanted.

I also think this is kind of a roundabout way to fix the issue. It prevents a non matching pattern from clearing out fields/tags by preventing empty values, but I think it would be better if we also directly fixed the core issue: when the pattern doesn't match it shouldn't modify the key/value at all (pending response from @44px).

@44px
Copy link
Contributor

44px commented Jul 6, 2018

Nope, I don't rely on this behavior. I added this because it seems more correct to emit empty string instead of original tag/field value when result_key specified and pattern doesn't match.

For example, having config like:

[[processors.regex]]
  [[processors.regex.fields]]
    key = "request"
    pattern = "^/api(?P<method>/[\\w/]+)\\S*"
    replacement = "${method}"
    result_key = "method"

And given request="/favicon.ico" it should emit method="", not method="/favicon.ico".

when the pattern doesn't match it shouldn't modify the key/value at all

You're right, it seems like more correct behavior. Plugin should not alter existing or emit new tag/field (depending on result_key) if there is no match. Here is my fix with new behavior: #4396

@danielnelson
Copy link
Contributor

Fixed in #4396

@glinton glinton deleted the bugfix/4374 branch July 17, 2018 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants